feat: add tailwind + shadcn/ui. #888
Conversation
styles.css
Outdated
| max-width: 1536px; | ||
| } | ||
| } | ||
| .visible { |
There was a problem hiding this comment.
should this file be checked in? It looks like the generated tailwind CSS file
src/styles/tailwind.css
Outdated
|
|
||
| */ | ||
|
|
||
| .button-container { |
src/styles/tailwind.css
Outdated
|
|
||
| @layer base { | ||
| :root { | ||
| --background: 0 0% 100%; |
There was a problem hiding this comment.
to make UI library compatible with obsidian plugin, we should use the CSS variables defined in obsidian (https://docs.obsidian.md/Reference/CSS+variables/CSS+variables). Many obsidian users use themes, which are made by altering those CSS variables (https://docs.obsidian.md/Themes/App+themes/Build+a+theme). Users would expect a well-designed plugin to change the appearance accordingly.
This is the main reason why I believe shadcn is the right choice for obsidian plugin UI library. It provides developer great flexibility to change how components are styled. However, we will lose the value if we use the shadcn default CSS variables.
I suggest:
- Do not introduce new CSS variables
- update tailwind.config.js to create tokens that map to obsidian CSS variables, including color, spacing, sizing, etc
- whenever we introduce a shadcn component, restyle it with the new token before we integrate it into the plugin.
There was a problem hiding this comment.
@zeroliu sometimes, I don't want to use some of the built-in theme styles.(eg: radius)
so, I think we should select some css styles to map.
|
I checked out this branch and ran locally, it seems to override some obsidian native CSS vars |
|
now, |
Is this going to be quite a big effort to make everything look the same/similar? I'm not familiar with how it can be converted though. @zeroliu is the expert here I'll wait for his recommendation. |
Emt-lin
force-pushed
the
UI
branch
2 times, most recently
from
e031a19 to
a848f01
Compare
I already delete |
package.json
Outdated
| "dev": "npm-run-all --parallel dev:*", | ||
| "dev:tailwind": "npx tailwindcss -i src/styles/tailwind.css -o ./styles.css --watch", | ||
| "dev:esbuild": "node esbuild.config.mjs", | ||
| "build:tailiwind": "npx tailwindcss -i src/styles/tailwind.css -o ./styles.css --minify", |
There was a problem hiding this comment.
Typo: build:tailiwind. I recommend some form of AI assistance to spot these things in the future. It's hard for reviewers to spot.
package.json
Outdated
| "dev:tailwind": "npx tailwindcss -i src/styles/tailwind.css -o ./styles.css --watch", | ||
| "dev:esbuild": "node esbuild.config.mjs", | ||
| "build:tailiwind": "npx tailwindcss -i src/styles/tailwind.css -o ./styles.css --minify", | ||
| "build": "npm run build:tailiwind && tsc -noEmit -skipLibCheck && node esbuild.config.mjs production", |
There was a problem hiding this comment.
How about something like
{
"scripts": {
"build:tailwind": "npx tailwindcss -i src/styles/tailwind.css -o ./styles.css --minify",
"build:esbuild": "tsc -noEmit -skipLibCheck && node esbuild.config.mjs production",
"build": "npm run build:tailwind && npm run build:esbuild || exit 1"
}
}
|
Awesome job! The UI now looks correct to me. One concern is that the bundle size is up significantly from 5MB+ to 21MB+. Is there any way we can make it smaller? Tailwind purging? |
| preflight: false, | ||
| }, | ||
| theme: { | ||
| extend: { |
There was a problem hiding this comment.
can we not extend but redefine all the tokens? Otherwise, people can still use predefined tailwind tokens without realizing that it's not connected to obsidian CSS variables. We can also turn on this eslint rule to enforce only supported classnames are used in the code base: https://github.com/francoismassart/eslint-plugin-tailwindcss/blob/master/docs/rules/no-custom-classname.md
| <input | ||
| type={type} | ||
| className={cn( | ||
| "flex h-9 w-full rounded-md border border-input bg-transparent px-3 py-1 text-base shadow-sm transition-colors file:border-0 file:bg-transparent file:text-sm file:font-medium file:text-foreground placeholder:text-muted-foreground focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring disabled:cursor-not-allowed disabled:opacity-50 md:text-sm", |
There was a problem hiding this comment.
do we support file type input? If not, I suggest not adding the related style until it's needed
|
|
||
| import { cn } from "@/lib/utils"; | ||
|
|
||
| const Input = React.forwardRef<HTMLInputElement, React.ComponentProps<"input">>( |
There was a problem hiding this comment.
not blocking this PR but maybe we can look into React 19 and use the ref props now that it's officially GA. Maintaining forwardRef for RadixUI can be pretty tedious.
No description provided.